Conversation
CodSpeed Performance ReportMerging #251 will not alter performanceComparing Summary
|
3b6c5ce to
2848551
Compare
bb67f16 to
730332e
Compare
| @@ -148,6 +154,7 @@ where | |||
| authorization_list: Default::default(), | |||
| }, | |||
| rlp_bytes: Some(Default::default()), | |||
There was a problem hiding this comment.
Not related to this PR, but could it cause any issues that tx.base.data and tx.rlp_bytes don't match?
| compressor.write_all(bytes.as_ref()).expect("failed to write bytes to compressor"); | ||
|
|
||
| // Finish the compression and get the result. | ||
| let result = compressor.finish().expect("failed to finish compression"); |
There was a problem hiding this comment.
Should we propagate error instead of panicking?
There was a problem hiding this comment.
This might be problematic, since from_encoded_tx and from_recovered_tx are not fallible... My understanding is that these should not fail in practice.
There was a problem hiding this comment.
This is the challenge I encountered. The BlockExecutor does not have a way to propagate errors, so at some point we will need to unwrap the error and panic. Given that we currently execute this function on already built blocks then it should not be possible to encounter a panic in practice. In the payload builder we may need to be more careful in case there is some way a user can manipulate the input transaction to result in a panic. However, I still believe this is unlikely.
| /// feature is not enabled. This is to support `no_std` environments where zstd is not | ||
| /// available. | ||
| pub fn compute_compression_ratio<T: AsRef<[u8]>>(_bytes: &T) -> U256 { | ||
| panic!("Compression feature is not enabled. Please enable the 'compression' feature to use this function."); |
There was a problem hiding this comment.
If we don't enable zstd_compression, what can we use these crates for? Won't it always run into this runtime panic?
There was a problem hiding this comment.
The ScrollBlockExecutor is the primary component used by the prover to prove blocks. We can not have zstd in the dependency chain for the prover as it is not compatible with riscv / openvm. As such, we need a means of instantiating the ScrollBlockExecutor in this context and that is the purpose of the zstd_compression feature flag. The prover will compute the compression ratios in the host where zstd is available and then provide them to the guest and execute the block using:
reth/crates/scroll/alloy/evm/src/block/mod.rs
Lines 97 to 132 in 89d2bf0
Given that we use let tx = tx.to_compressed(compression_ratio); to instantiate the WithCompressed type, the compute_compression_ratio function will never be invoked.
Overview
This PR introduces support for transaction compression. It does so with the introduction of the
WithCompressiontype:This allows for a
compression_ratioto be associated with a transaction. As the currentzstdlibrary is not compatible withno_stdwe panic if we try to compress a transaction in ano_stdenvironment to provideno_stdsupport.reth/crates/scroll/alloy/evm/src/tx/compression.rs
Lines 71 to 81 in 6a86b0d
We should migrate to
zstd-safesuch that we can achieveno-stdsupport natively without such a panic.We expose a function to compute compression ratios (
compute_compression_ratio):reth/crates/scroll/alloy/evm/src/tx/compression.rs
Lines 44 to 68 in 6a86b0d
The compression ratios for a transaction can be computed in a
stdenvironment and then provided to theScrollBlockExecutorvia:reth/crates/scroll/alloy/evm/src/block/mod.rs
Lines 108 to 128 in 6a86b0d
Future work:
We should modify the pooled transaction type to compute the compression factor only once:
reth/crates/scroll/txpool/src/transaction.rs
Lines 15 to 48 in 6a86b0d